feat(wallet): add ability to send a token #18242
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (51)
|
J-Son89
requested review from
smohamedjavid,
ulisesmac,
vkjr,
mmilad75 and
shivekkhurana
|
I can't figure out what it is, but the floating button page is not sitting right for slide button. 
If I hot reload it sits perfectly though 😅 If anyone has any ideas what the fix is would be appreciated |
J-Son89
commented
Dec 20, 2023
| :amount (str ethereum " ETH") | ||
| :divider? (or show-arbitrum? show-optimism?) | ||
| :theme theme}]) | ||
| (when show-optimism? |
There was a problem hiding this comment.
the designs only show these if they have a value. We could also look to remove network specific details out of this component and make it more generic but it's for another pr as this work is already growing too large.
J-Son89
commented
Dec 20, 2023
| [account-avatar/view account-props] | ||
| (= type :saved-account) | ||
| [wallet-user-avatar/wallet-user-avatar (assoc account-props :size :size-32)] | ||
| (= type :account) |
There was a problem hiding this comment.
some slight differences for designs then what was initially implemented here. I think it went unnoticed and will ask for a design review for this component.
J-Son89
commented
Dec 20, 2023
| container-style]}] | ||
| [rn/view {:style {:flex 1}} | ||
| container-style | ||
| ] |
There was a problem hiding this comment.
will adjust formatting 🤒
J-Son89
commented
Dec 20, 2023
| (assoc account-props | ||
| :size :size-32 | ||
| :neutral? true | ||
| )] |
There was a problem hiding this comment.
will adjust formatting 🤒
J-Son89
commented
Dec 20, 2023
J-Son89
commented
Dec 20, 2023
| amount (:amount send-transaction-data) | ||
| route (:route send-transaction-data) | ||
| estimated-time-min (:EstimatedTime route) | ||
| max-fees "-" |
There was a problem hiding this comment.
max fees to be calculated properly in a follow up 👍
OmarBasem
reviewed
Dec 20, 2023
| (:GasFees | ||
| route)))) | ||
| :Input "" | ||
| :Data "0x"}}] |
There was a problem hiding this comment.
What is that hardcoded "0x" ?
There was a problem hiding this comment.
I will find the explanation and send it on. For now this is okay to leave as is though.
There was a problem hiding this comment.
Data is a secondary feature, it is something which can be useful when making something other than an ETH transaction. It is the encoding of the contract call. We will eventually follow up to include this feature.
cc @alaibe in case I left out any details
| token-symbol (:symbol token) | ||
| amount (:amount send-transaction-data) | ||
| route (:route send-transaction-data) | ||
| estimated-time-min (:EstimatedTime route) |
There was a problem hiding this comment.
Can the keys in this data be converted to kebab-case (:EstimatedTime route)
There was a problem hiding this comment.
yeah you're right, will adjust these!
ulisesmac
reviewed
Dec 20, 2023
| (when (pos? ethereum) | ||
| [network-amount | ||
| {:network :ethereum | ||
| :amount (str ethereum " ETH") |
There was a problem hiding this comment.
Do we have a place to extract these strings ("ETH", "OPT", etc) by network name (e.g. :optimism) ?
There was a problem hiding this comment.
I would like to revisit how this component is working - I will create a follow up issue for this. For this pr it goes outside of the scope and send already is a huge feature set. imo the quo components should have no knowledge of networks and should just render the UI.
| (js/alert "Not implemented yet") | ||
| {:db (assoc-in db [:wallet :ui :send :amount] amount)})) | ||
| (fn [{:keys [db]} [{:keys [amount stack-id]}]] | ||
| {:db (assoc-in db [:wallet :ui :send :amount] amount) |
Comment on lines
105
to
164
| (rf/reg-event-fx :wallet/send-transaction | ||
| (fn [{:keys [db]} [sha3-pwd]] | ||
| (let [route (get-in db [:wallet :ui :send :route]) | ||
| from-address (get-in db [:wallet :current-viewing-account-address]) | ||
| to-address (get-in db [:wallet :ui :send :to-address]) | ||
| from (:From route) | ||
| token (get-in db [:wallet :ui :send :token]) | ||
| token-id (:symbol token) | ||
| from-asset token-id | ||
| to-asset token-id | ||
| bridge-name (:BridgeName route) | ||
| chain-id (:chainId from) | ||
| multi-transaction-command {:fromAddress from-address | ||
| :toAddress to-address | ||
| :fromAsset from-asset | ||
| :toAsset to-asset | ||
| :fromAmount (:AmountOut route) | ||
| :type 0} | ||
| transaction-bridge | ||
| [{:BridgeName bridge-name | ||
| :ChainID chain-id | ||
| :TransferTx {:From from-address | ||
| :To to-address | ||
| :Gas (money/to-hex (:GasAmount route)) | ||
| :GasPrice (money/to-hex (money/->wei :gwei | ||
| (:gasPrice (:GasFees route)))) | ||
| :Value (:AmountOut route) | ||
| :Nonce nil | ||
| :MaxFeePerGas (money/to-hex | ||
| (money/->wei :gwei | ||
| (:maxFeePerGasMedium (:GasFees route)))) | ||
| :MaxPriorityFeePerGas (money/to-hex (money/->wei :gwei | ||
| (:maxPriorityFeePerGas | ||
| (:GasFees | ||
| route)))) | ||
| :Input "" | ||
| :Data "0x"}}] | ||
| request-params [multi-transaction-command transaction-bridge sha3-pwd]] | ||
| {:json-rpc/call [{:method "wallet_createMultiTransaction" | ||
| :params request-params | ||
| :on-success (fn [result] | ||
| (rf/dispatch [:hide-bottom-sheet]) | ||
| (rf/dispatch [:wallet/add-authorized-transaction result])) | ||
| :on-error (fn [error] | ||
| (log/error "failed to send transaction" | ||
| {:event :wallet/send-transaction | ||
| :error error | ||
| :params request-params}))}]}))) |
There was a problem hiding this comment.
This event looks a little large. I think we could split it into more small pieces.
Most of the problems related to why it is verbose are related to CamelCase, so I'd suggest to transform the keys keys while being stored:
(rf/reg-event-fx
:wallet/suggested-routes-success
(fn [{:keys [db]} [suggested-routes timestamp]]
(when (= (get-in db [:wallet :ui :send :suggested-routes-call-timestamp]) timestamp)
{:db (-> db
(assoc-in [:wallet :ui :send :suggested-routes] suggested-routes)
;; Transform the keys in `(first (:Best suggested-routes))` to kebab case
(assoc-in [:wallet :ui :send :route] (first (:Best suggested-routes)))
(assoc-in [:wallet :ui :send :loading-suggested-routes?] false))})))That will also solve the problem @OmarBasem said about transforming the estimated time route.
Once that is done, we could refactor this event easier, just as a suggestion:
(rf/reg-event-fx
:wallet/send-transaction
(fn [{{wallet-db :wallet} :db} [sha3-pwd]]
(let [{:keys [route token
to-address]} (get-in wallet-db [:ui :send])
from-address (:current-viewing-account-address wallet-db)
token-id (:symbol token)
multi-transaction-command {:fromAddress from-address
:toAddress to-address
:fromAsset token-id
:toAsset token-id
:fromAmount (:amount-out route)
:type 0}
transaction-bridge (get-transaction-bridge {:from-address from-address
:to-address to-address
:route route})
request-params [multi-transaction-command transaction-bridge sha3-pwd]]
{:json-rpc/call [,,,]})))That is getting the transaction bridge map in a separate function. I'm not sure rn, but I think getting a transaction bridge will be a common operation, so maybe that function can be reused or extended in the future. ATM, it just helps to readability, that function is:
(defn- get-transaction-bridge [{:keys [from-address to-address route]}]
(let [{:keys [from bridge-name amount-out gas-amount gas-fees]} route
{:keys [gas-price max-fee-per-gas-medium max-priority-fee-per-gas]} gas-fees]
[{:BridgeName bridge-name
:ChainID (:chain-id from)
:TransferTx {:From from-address
:To to-address
:Gas (money/to-hex gas-amount)
:GasPrice (money/to-hex (money/->wei :gwei gas-price))
:Value amount-out
:Nonce nil
:MaxFeePerGas (money/to-hex (money/->wei :gwei max-fee-per-gas-medium))
:MaxPriorityFeePerGas (money/to-hex (money/->wei :gwei max-priority-fee-per-gas))
:Input ""
:Data "0x"}}]))There was a problem hiding this comment.
Yep, I agree with everything here and about the keys neededing to be adjusted. Will do all! Thanks for the feedback @ulisesmac & @OmarBasem :)
Comment on lines
+96
to
+105
| (let [transaction-hashes (:hashes transaction) | ||
| chain-id (key (first transaction-hashes)) | ||
| tx-id (first (val (first transaction-hashes))) |
There was a problem hiding this comment.
Why do we only get the first item? what is contained in the rest of them?
There was a problem hiding this comment.
ah this is a temporary solution, will adjust properly when we go to implement the transaction progress page.
Happy to remove it for now, I think I left it there to also open up discussion about how we store transactions in the db
There was a problem hiding this comment.
Discussed with @ulisesmac - will create an issue for this feature and mention this needs to be reworked. 👍
Comment on lines
70
to
72
| :values {:ethereum amount | ||
| :optimism 0 | ||
| :arbitrum 0} |
There was a problem hiding this comment.
Are we setting the remaining networks as zero because we are not supporting them rn?
If so, maybe we can remove them or add a comment specifying this is temporal.
There was a problem hiding this comment.
I need to remove this and this should be pulled from data 👍
ulisesmac
reviewed
Dec 20, 2023
| transaction-detes {:status :pending | ||
| :id (:id transaction) | ||
| :chain-id chain-id}] | ||
| {:db (assoc-in db [:wallet :transactions tx-id] transaction-detes) |
There was a problem hiding this comment.
Just a question here,
Are transactions associated to an account?
If so, please consider adding these transactions inside the respective account
There was a problem hiding this comment.
let's discuss this today!
J-Son89
commented
Dec 21, 2023
| (keyword? token) name | ||
| :always (comp string/lower-case str))] | ||
| (get tokens token-symbol))) | ||
| (when token |
There was a problem hiding this comment.
@ulisesmac and I revisted this as we realised it was breaking the token component 👍
J-Son89
commented
Dec 21, 2023
| [rn/view | ||
| {:style (token-style {:justify-content :center | ||
| {:style (token-style (merge {:justify-content :center |
There was a problem hiding this comment.
@ulisesmac & I found a bug where empty tokens didn't sit right, this fix that 👍
J-Son89
commented
Dec 21, 2023
| (fn [{:keys [db]} [tab]] | ||
| {:db (assoc-in db [:wallet :ui :send :select-address-tab] tab)})) | ||
| (fn [{:keys [db]} [tab]] | ||
|
|
There was a problem hiding this comment.
will delete empty line 👍
J-Son89
commented
Dec 21, 2023
| (assoc-in [:wallet :ui :send :loading-suggested-routes?] false))}))) | ||
| (fn [{:keys [db]} [suggested-routes timestamp]] | ||
| (when (= (get-in db [:wallet :ui :send :suggested-routes-call-timestamp]) timestamp) | ||
| (let [suggested-routes-data (cske/transform-keys csk/->kebab-case suggested-routes) |
There was a problem hiding this comment.
lowered routes keys 👍
J-Son89
commented
Dec 21, 2023
| {:db (assoc-in db [:wallet :transactions tx-id] transaction-detes) | ||
| :fx [[:dispatch [:navigate-to :wallet-transaction-progress]]]}))) | ||
|
|
||
| (defn- transaction-bridge [{:keys [from-address to-address route]}] |
There was a problem hiding this comment.
put it in a small helper function 👍
J-Son89
force-pushed
the
jc/send-it
branch
3 times, most recently
from
1062580 to
a1cb0a1
Compare
|
@J-Son89 thank you for the PR. Speaking about happy flow - I was able to send some amount of EHT from Account A to Account B. Besides there are plenty issues, you have already mentioned some of them. I don't know if it makes sense describe all the issues, as you have pointed out that this flow is still in progress. I will mention some of them below so you can keep an eye on them in the follow up PRs. ISSUE 1 Not able to confirm sending STT (Status Test Token). Confirm button just does not respond.telegram-cloud-document-2-5238023543039475337.mp4ISSUE 2 Unable to return to wallet screen after completing the transactionOn IOS at the end user appears on the empty wallet screen. Need to relogin the app to unblock navigation to other screens. IOS Video telegram-cloud-document-2-5238023543039475331.mp4On Android not able to close Sending screen Android video: telegram-cloud-document-2-5238023543039475613.mp4ISSUE 3 Balance is not updated for token receiver until reloginISSUE 4 Balance is rounded to 2 decimals on “Select asset” screen.
ISSUE 5 “Doesn't support name” error when selecting ETH token on “Select asset” screen.
Cannot reproduce constantly. I have faced it only couple of times so far. |
@J-Son89 do I understand correctly that this PR needs design review based on this comment? |
|
@pavloburykh - thanks for all the reviews and mentioning those issues. I will check all these and create the issues as needed! and also just to note that particular component is not used any where else in the codebase (except for the preview screen) |
|
@J-Son89 thanks! So let's wait for Francesca's review. Meanwhile I have triggered e2e re-run as there were some failures in the previous run (most of them are 100% PR related though). I will provide feedback when the results are ready. Based on the e2e runs queue the results will be ready in couple of hours, so hopefully will be ready to merge the PR tomorrow. Sorry once again for the delay with testing Jamie. |
|
Not at all, thanks for the rigorous testing! Really helps put the feature together 🙏 |
56% of end-end tests have passedFailed tests (19)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (27)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
|
79% of end-end tests have passedFailed tests (8)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (38)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
|
|
@J-Son89 failed e2e are not PR related. So we are ready to merge after design review. |
Francesca-G
reviewed
Jan 5, 2024
There was a problem hiding this comment.
Design Review:
Quo Component - Tags -> Summary Tag was also revisited as it was missng some designs. It would be good to check this component is looking alright here 👍
Here's the design review for the Summary Tag component :)
https://www.figma.com/file/Tf5nfkYvpbnNCo4rKLK7lS/Feedback-for-Mobile?type=design&node-id=3415%3A214763&mode=design&t=ANYgMF9xQ4Gp2lOW-1
|
Thanks for the review @Francesca-G - I will create a separate issue to make those adjustments. 🙏 |
|
@pavloburykh - I tracked these issues: Issue 2 - will be done as part of Issue 3: #18412 Issue 4: #18414 Issue 5- @Francesca-G - design review updates tracked here: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes: #18005
This pr adds the integration with Status Go to send a transaction.
Testing Notes:
This pr is adding the happy path for sending a token.
To send a token this is the flow.
This is only for the mainnet transactions (including testnet), support for other Optimism & Arbitrum will be added in addtional pr's.
Transaction Confirmation page is missing a few UI details, such as gas, time of transaction etc. I will create a follow up issue to implement these completely.
Transaction progress page is only initial state of it, there is no expected functionality there as this will be handled in a separate issue. For now the button on that page will navigate to the account-page.
This navigation should also end up with the Transactions tab active on the send page, this is beyond the scope of this pr. The main focus is to be able to send any token
The slide button on the transaction confirmation page is not sitting right. This is a common component and I would prefer to handle this issue as I don't want to block this pr because of that. The send flows are high priority for 1.27 and this issue can be handled in isolation 👍
Design Review:
Quo Component - Tags -> Summary Tag was also revisited as it was missng some designs. It would be good to check this component is looking alright here 👍
Screen.Recording.2023-12-21.at.20.29.57.mov